Conversation
ghost
added
Area-Analyzers
untriaged
dotnet-policy-service
bot
added
the
Needs API Review
jjonescz
changed the title
IAnalyzerAssemblyResolver public API
|
@RikkiGibson @jaredpar for reviews |
| namespace Microsoft.CodeAnalysis.Workspaces.AnalyzerRedirecting; | ||
|
|
||
| /// <summary> | ||
| /// Any MEF component implementing this interface will be used to redirect analyzer assemblies. |
There was a problem hiding this comment.
Can we add a bit here that drives home this happens before shadow copy? Essentially this redirects the path that we pass to the compiler and the compiler will then do as it normally does with the path?
|
@dotnet/roslyn-ide, @jasonmalinowski PTAL |
|
@jasonmalinowski for a review, thanks |
|
@jasonmalinowski PTAL |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
Some comments. The code itself is fine, but after the discussion I had with @jaredpar I'm not entirely convinced this can be implemented as the interface is currently structured given some of the async requirements that exist in VS and the stronger requirements that exist in Roslyn. Do we already have an implementation of this that I could look at?
| /// or <see langword="null"/> if this instance cannot redirect the given assembly. | ||
| /// </returns> | ||
| /// <remarks> | ||
| /// If two redirects return different paths for the same assembly, no redirection will be performed. |
There was a problem hiding this comment.
| /// If two redirects return different paths for the same assembly, no redirection will be performed. | |
| /// If two redirectors return different paths for the same assembly, no redirection will be performed. |
|
|
||
| private string? TryRedirectAnalyzerAssembly(string fullPath) | ||
| { | ||
| if (!_hostInfo.AnalyzerAssemblyRedirectors.IsDefaultOrEmpty) |
There was a problem hiding this comment.
Not sure this is strictly necessary -- the foreach will bail quite quickly if this is the case...
| } | ||
| } | ||
| } | ||
| catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.Diagnostic)) |
There was a problem hiding this comment.
| catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.Diagnostic)) | |
| catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.General)) |
Or maybe even critical. Because if a redirector is throwing exceptions, then we're likely loading mismatched binaries and the product might be broken anyways.
| return GetMappedRazorSourceGenerator(fullPath); | ||
| } | ||
|
|
||
| if (TryRedirectAnalyzerAssembly(fullPath) is { } redirectedPath) |
There was a problem hiding this comment.
If @jaredpar's work doesn't come first, we should then refactor this method away by converting the IsSdkRazorSourceGenerator/GetMappedRazorSourceGenerator methods into a redirector.
There was a problem hiding this comment.
Yeah, I think Jared's work is going to refactor all this and we just want to get this in so we can continue on the SDK side etc.
| <RestrictedInternalsVisibleTo Include="Microsoft.VisualStudio.IntelliCode" Partner="Pythia" Key="$(IntelliCodeKey)" /> | ||
| <RestrictedInternalsVisibleTo Include="Microsoft.VisualStudio.IntelliCode.CSharp" Partner="Pythia" Key="$(IntelliCodeCSharpKey)" /> | ||
| <RestrictedInternalsVisibleTo Include="Microsoft.VisualStudio.IntelliCode.CSharp.Extraction" Partner="Pythia" Key="$(IntelliCodeCSharpKey)" /> | ||
| <RestrictedInternalsVisibleTo Include="Microsoft.Net.Sdk.AnalyzerRedirecting" Namespace="Microsoft.CodeAnalysis.Workspaces.AnalyzerRedirecting" /> |
There was a problem hiding this comment.
Just to be 100% clear here, this Microsoft.Net.Sdk.AnalyzerRedirecting assembly is something that ships with VS, and is versioned with VS, and does not ship inside the .NET SDK?
There was a problem hiding this comment.
It is built and versioned from the .NET SDK repo, then inserted into VS and shipped with VS.
| /// <remarks> | ||
| /// If two redirects return different paths for the same assembly, no redirection will be performed. | ||
| /// </remarks> | ||
| string? RedirectPath(string fullPath); |
There was a problem hiding this comment.
Would this ever need to be async? Or is the assumption that any async would be to create the redirector in the first place?
There was a problem hiding this comment.
Haven't thought about that. Currently, no async is needed. When we need async during creation or redirecting, we can update the code, I think. We have just one implementation and don't expect to have more for now.
| namespace Microsoft.CodeAnalysis.Workspaces.AnalyzerRedirecting; | ||
|
|
||
| /// <summary> | ||
| /// Any MEF component implementing this interface will be used to redirect analyzer assemblies. |
There was a problem hiding this comment.
This should have a comment explicitly stating what threading expectations exist here. Since it's not async that means no thread switching would be allowed. (And if it was async, it might still require the stronger guarantee that no thread switching is allowed.)
Per the conversation I had with @jaredpar earlier today, I'm not entirely convinced this interface could actually be implemented safely, especially if to actually implement it you need to fetch any VS service, which would require asynchrony.
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
Digging further I found dotnet/sdk#42861 as the implementation, which answered a few of my questions. So that works fine because the implementation isn't needing to grab any VS services. (And it's doing so by looking at AppDomain.CurrentDomain.BasePath which seems...highly suspect long term) So although this works fine for that implementation I'm not sure any other implementations would be able to use this, and I don't think we could migrate the Razor code over to use this if we wanted to without having to revise the API shape.
@jaredpar given you had some other refactorings coming, thoughts on this?
Hm, I just need to find path to VS (because that's where the runtime analyzers are deployed and we want to redirect to them). Is there a better way?
I don't think it's the plan to have more implementations, this is merely a hook for the SDK. |
7 participants
Uh oh!
There was an error while loading. Please reload this page.